Skip to content

fix: strengthen upgrade tests and show removed packages in UX#3630

Open
itsjustriley wants to merge 4 commits intomainfrom
fix/cli-upgrade-logic
Open

fix: strengthen upgrade tests and show removed packages in UX#3630
itsjustriley wants to merge 4 commits intomainfrom
fix/cli-upgrade-logic

Conversation

@itsjustriley
Copy link
Copy Markdown
Contributor

@itsjustriley itsjustriley commented Mar 26, 2026

WHY are these changes introduced?

Fixes https://github.com/Shopify/developer-tools-team/issues/1113
Fixes https://github.com/Shopify/developer-tools-team/issues/1138
Fixes https://github.com/Shopify/developer-tools-team/issues/1107

Upgrade command tests had weak assertions that could pass vacuously, changelog validation coverage was lost when tests were deleted, and the upgrade UX didn't surface removed packages.

WHAT is this pull request doing?

  • Strengthen renderTasks assertions: Verify correct task titles instead of just toHaveBeenCalled(); verify removal task is absent when no deps to remove
  • Port deleted changelog validation tests: ~270 lines from upgrade-flow.test.ts into new changelog-schema.test.ts — validates allowed fields, URL/version/date format, semver deps, base64 steps, dependenciesMeta
  • Show removed packages in upgrade UX: New "Removed packages" section in displayConfirmation and markdown list in generateUpgradeInstructionsFile. Uses extracted getAllRemovedPackages() helper to avoid duplication.

HOW to test your changes?

  • npx vitest run packages/cli/src/commands/hydrogen/upgrade.test.ts --pool=forks --poolOptions.forks.singleFork
  • npx vitest run packages/cli/src/commands/hydrogen/changelog-schema.test.ts --pool=forks --poolOptions.forks.singleFork

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

@shopify
Copy link
Copy Markdown
Contributor

shopify bot commented Mar 26, 2026

Oxygen deployed a preview of your fix/cli-upgrade-logic branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
Skeleton (skeleton.hydrogen.shop) ✅ Successful (Logs) Preview deployment Inspect deployment April 15, 2026 2:28 PM

Learn more about Hydrogen's GitHub integration.

@itsjustriley itsjustriley force-pushed the fix/cli-upgrade-logic branch from bbaafb4 to 3432021 Compare March 27, 2026 13:58
@itsjustriley
Copy link
Copy Markdown
Contributor Author

CI failure unrelated to this PR

@itsjustriley itsjustriley marked this pull request as ready for review March 27, 2026 14:13
@itsjustriley itsjustriley requested a review from a team as a code owner March 27, 2026 14:13
Comment thread packages/cli/src/commands/hydrogen/upgrade.test.ts
Comment thread packages/cli/src/commands/hydrogen/upgrade.test.ts
Comment thread packages/cli/src/commands/hydrogen/upgrade.test.ts Outdated
Comment thread .changeset/removed-packages-ux.md Outdated
Comment thread packages/cli/src/commands/hydrogen/upgrade.ts
Comment thread packages/cli/src/commands/hydrogen/upgrade.ts
Comment thread packages/cli/src/commands/hydrogen/upgrade.test.ts
Comment thread packages/cli/src/commands/hydrogen/changelog-schema.test.ts Outdated
Comment thread packages/cli/src/commands/hydrogen/changelog-schema.test.ts Outdated
@fredericoo
Copy link
Copy Markdown
Contributor

couple unit tests are failing, maybe try merging/rebasing main back in? i think we fixed those a lil bit ago

@itsjustriley itsjustriley force-pushed the fix/cli-upgrade-logic branch from 9540a0e to d6a9eb7 Compare April 15, 2026 14:20
itsjustriley and others added 4 commits April 15, 2026 11:26
Merchants upgrading across versions that remove packages (e.g. the
Remix → React Router migration) had no visibility into what was being
cleaned up. Now they see a "Removed packages" section in both the
upgrade confirmation prompt and the generated instructions markdown.

- Extract getAllRemovedPackages() helper to avoid duplicating the
  removeDependencies + removeDevDependencies concatenation across
  displayConfirmation and generateUpgradeInstructionsFile
- Extend displayConfirmation to show removed packages list when present
- Extend generateUpgradeInstructionsFile to include a "## Removed
  packages" section with backtick-wrapped package names
- Export generateUpgradeInstructionsFile for testability

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The upgrade command tests had weak assertions that could pass vacuously,
and changelog validation coverage was lost when tests were deleted from
upgrade-flow.test.ts.

Upgrade test improvements:
- Verify renderTasks receives specific task titles instead of just
  toHaveBeenCalled()
- Assert removal task is absent when no deps to remove (direct mock
  inspection pattern)
- Add tests for removed packages display in confirmation prompt and
  generated markdown instructions

New changelog-schema.test.ts (~290 lines):
- Validates changelog.json schema: allowed fields, required fields,
  URL/version/date formats, semver deps, base64 steps, dependenciesMeta
- Uses semver.validRange() instead of a permissive regex for version
  validation — the regex accepted trailing garbage like 1.0.0!@#$%
- Precondition assertions on required-field loops prevent vacuous passes
  (e.g. if all releases lost their features/fixes arrays, the test would
  still pass green without the precondition)
- Optional fields (dependenciesMeta, removeDependencies) intentionally
  have no preconditions — a valid changelog may have zero instances

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Replace fragile `mock.calls[0]?.[0]` traversal with
  `expect.not.arrayContaining` per kdaviduik's suggestion; the
  built-in matcher is resilient to call-order changes and removes
  the need for the inline `{title: string}` cast
- Add negative test for `generateUpgradeInstructionsFile` asserting
  the "Removed packages" section is omitted when there are no removals
- Add edge-case test for both `displayConfirmation` and
  `generateUpgradeInstructionsFile` when a release has only removals
  and no features/fixes — covering the new UX path noted by fredericoo
- Extract `assertDefined<T>` type assertion helper in
  `changelog-schema.test.ts` to replace the 6x repeated
  `expect(release).toBeDefined(); if (!release) continue;` pattern;
  after calling `assertDefined`, TypeScript narrows the type so the
  manual guard is no longer needed
- Hoist `getChangelog()` into `beforeAll` in `changelog-schema.test.ts`
  to parse the static JSON once instead of once per test (~9 calls)
- Update changeset wording per kdaviduik's nit

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When `allRemovedPackages` is present but `fixes` is absent, the prior
section (h1/breaking/features) already ends with `\n----\n`. The old
code unconditionally prepended `\n----\n\n` before "## Removed packages",
producing a `----\n\n----` double horizontal rule in the output.

Fix: conditionally add the separator only when `fixesMd.length > 0`
(fixes emits no trailing separator, so we supply one) vs. `'\n'` when
fixes is absent (prior section already terminated with `----\n`).

Also replace silent `if (!item) continue` / `if (!step) continue`
guards in changelog-schema.test.ts with `assertDefined()` so null/
undefined entries in changelog.json cause an explicit test failure
rather than a silent skip that masks malformed data.
@itsjustriley itsjustriley force-pushed the fix/cli-upgrade-logic branch from d6a9eb7 to 1198f9c Compare April 15, 2026 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants